KAFKA-18559: Cleanup FinalizedFeatures #18593
Conversation
github-actions
bot
added
triage
There was a problem hiding this comment.
@frankvicky thanks for this patch. please take a look at one small comment.
| this.metadataVersion = metadataVersion; | ||
| this.finalizedFeatures = new HashMap<>(finalizedFeatures); | ||
| this.finalizedFeaturesEpoch = finalizedFeaturesEpoch; | ||
| // In KRaft mode, we always include the metadata version in the features map. |
There was a problem hiding this comment.
Could you please keep the comment "In KRaft mode, we always include the metadata version in the features map"?
There was a problem hiding this comment.
done.
To avoid confusion, I delete the in kraft.
frankvicky
force-pushed
the
KAFKA-18559
branch
from
826cc54 to
0f7db63
Compare
| ) { | ||
| Objects.requireNonNull(metadataVersion); | ||
| this.metadataVersion = metadataVersion; |
There was a problem hiding this comment.
Could you please merge them?
this.metadataVersion = Objects.requireNonNull(metadataVersion);There was a problem hiding this comment.
Sure!
I have just updated the patch
frankvicky
force-pushed
the
KAFKA-18559
branch
3 times, most recently
from
b7a6b06 to
a7c06fe
Compare
| } else { | ||
| this.finalizedFeatures.remove(MetadataVersion.FEATURE_NAME); | ||
| } | ||
| // Intentionally include metadata version in features map for version consistency |
There was a problem hiding this comment.
This comment is a bit confusing to me - what are we trying to say?
There was a problem hiding this comment.
Hi @ijuma,
Thanks for the review.
Previously, I simply deleted // In KRaft mode from the comment, but I realized this could be confusing. Then I tried rephrasing it, but it seems even more confusing now. 😓
I'm wondering if we should delete the entire comment instead?
The original comment was to highlight the difference between zk and kraft behavior. Now that zk is gone, do we still need this comment?
There was a problem hiding this comment.
We might want to revisit all callers to double-check if this is still a requirement.
frankvicky
force-pushed
the
KAFKA-18559
branch
from
a7c06fe to
2ebd404
Compare
JIRA: KAFKA-18559 Cleanup the zk logic and test in `FinalizedFeatures`
frankvicky
force-pushed
the
KAFKA-18559
branch
from
2ebd404 to
cafd9b9
Compare
|
the failed test is traced by https://issues.apache.org/jira/browse/KAFKA-18441 |
Reviewers: Ismael Juma <[email protected]>, Chia-Ping Tsai <[email protected]>
Reviewers: Ismael Juma <[email protected]>, Chia-Ping Tsai <[email protected]>
JIRA: KAFKA-18559
Cleanup the zk logic and test in
FinalizedFeaturesCommitter Checklist (excluded from commit message)